Print a message on OOM #30801

Amanieu · 2016-01-09T19:38:36Z

This adds the ability to override the default OOM behavior by setting a handler function. This is used by libstd to print a message when running out of memory instead of crashing with an obscure "illegal hardware instruction" error (at least on Linux).

Fixes #14674

rust-highfive · 2016-01-09T19:38:51Z

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

hanna-kruppe · 2016-01-09T19:52:36Z

Does this actually work? That is, does rtabort! guarantee that it doesn't allocate memory?

Amanieu · 2016-01-09T19:59:50Z

I checked that the implementation of rtabort! doesn't allocate memory on both windows and unix. I also ran a quick test and the message was correctly printed when the allocator failed to allocate memory.

Amanieu · 2016-01-09T20:01:40Z

Actually, I'll take that back, it seems that rtabort! does allocate memory on windows for UTF-16 conversion.

Amanieu · 2016-01-09T20:24:47Z

Since this is mainly going to be useful on unix platforms (windows will display a message box from the abort), I could simply make it unix-only where rtabort! is known to never allocate memory.

Amanieu · 2016-01-09T21:09:19Z

I instead made the OOM handler reset itself once it is invoked so that any recursive failure to allocate memory will simply fall back to the old behavior of aborting without a message.

retep998 · 2016-01-10T19:46:33Z

Even on Windows it would be nice to have a message because often times when running a program in the console, the message box doesn't pop up, and even when it does it doesn't tell you anything useful other than the program crashed. Since most of the time people will hit oom from accidentally passing a silly huge number number to Vec, I think we should at least try to print a message. The handler resetting itself seems like a reasonable solution.

mmcco · 2016-01-10T20:40:49Z

Couldn't the error string just be stored as a static?

retep998 · 2016-01-10T21:09:50Z

@mmcco Sure, you could have a const array of u16 and write that directly. If that is unreasonable since the message is ascii you could also just cast the u8 codeunits to u16 and store it in a buffer on the stack and write that.

alexcrichton · 2016-01-11T23:22:38Z

@Amanieu yes as you've found this will allocate memory on Windows, I think for two reasons:

If we fail to get a handle to the stderr stream, then creating the error to be returned will allocate memory.
When printing we'll allocate memory in the utf16 conversion

@retep998 do you know of a guaranteed way to print to the console which is infallible? If so, we could use that vector for printing information.

Otherwise I do agree that resetting the handler to the default "abort and die" seems reasonable right before we attempt to print. Note that this has implications on the multithreaded scenario, however, where if one thread is attempting to print and another hits OOM it'll take down the process without printing anything anyway. I believe that may be fixable by having a global mutex around the OOM handler, but that itself will be tricky to do.

I'm a little hesitant about this as it's very tough to ensure there are 0 allocations along the way in the oom handler unless it's basically written from scratch. I would almost prefer that the handler in libstd is written in such a way that all the code is self-contained to be easily inspected for "this doesn't allocate". It'll be a little uglier, but much easier to audit and future-proof from accidental modifications

Amanieu · 2016-01-11T23:31:34Z

One possibility would be to call libc::write directly on file descriptor 2. This would work on all platforms and avoid allocating memory, but it goes against the goal of having libstd not depend on libc for Windows.

retep998 · 2016-01-11T23:59:17Z

@alexcrichton Remove the layers of abstraction? Just get GetStdHandle(STD_ERROR_HANDLE), and call WriteFile on it with a simple ascii string. You don't even need to bother checking whether GetStdHandle returned a valid handle nor do you need to check the return value of WriteFile since WriteFile is memory safe with regard to invalid handles and you're going to terminate anyway.

Amanieu · 2016-01-12T00:35:38Z

I implemented a new version which uses separate platform-specific OOM handlers for windows & unix. I only tested the unix version, so there may be issues with the windows code.

Here's a small test program which triggers an OOM: http://ix.io/nkx You may have to adjust the numbers if you are running on a 32-bit system.

retep998 · 2016-01-12T00:41:33Z

If that windows code does compile, then I'm pretty sure it'll work correctly.

alexcrichton · 2016-01-12T01:33:25Z

@Amanieu yeah that's what I had in mind, I'll take a look.

@retep998 yes I'm aware the layers can be removed, I was wondering if there was an infallible method because GetStdHandle can fail.

alexcrichton · 2016-01-12T01:35:34Z

src/liballoc/oom.rs

+pub fn oom() -> ! {
+    // Reset the OOM handler to avoid infinite loops if the OOM handler tries
+    // to allocate memory and fails.
+    let value = OOM_HANDLER.swap(default_oom_handler as *mut (), Ordering::SeqCst);


I think that with the changes below it should be fine to have this just be load instead of swap. I'm worried about the usefulness of the swap here because if a bunch of threads hit OOM at the same time the process is likely to terminate without printing anything (the second thread will abort the process likely before the first prints).

The implementations below are pretty safe to audit for not having any allocations in them as well, I believe.

Amanieu · 2016-01-12T01:36:14Z

If GetStdHandle fails then there is nothing attached to stderr, so printing a message is pointless anyways.

alexcrichton · 2016-01-12T01:36:53Z

src/libstd/sys/unix/mod.rs

@@ -58,6 +59,20 @@ pub fn init() {
    unsafe {
        assert!(signal(libc::SIGPIPE, libc::SIG_IGN) != !0);
    }
+
+    // A nicer handler for out-of-memory situations than the default one. This
+    // one prints a message to stderr before aborting.


Could you expand this to explain:

It's critical that this function doesn't have any allocations, so it shouldn't be using many abstractions.

Errors are ignored here because there's not really anything else that can be done.

alexcrichton · 2016-01-12T01:39:09Z

Looking good to me, thanks @Amanieu!

Amanieu · 2016-01-12T01:54:06Z

Updated according to feedback.

retep998 · 2016-01-12T02:01:08Z

If GetStdHandle does fail then you're pretty much out of luck. It is the way to get stderr and everything else including libc uses it, and would only "fail" in the rare case that an invalid handle was assigned to stderr in which case it is the parent process's fault or some library's fault for setting it to an invalid handle. If someone wants to see the out of memory message, just like if they'd see the panic messages, they need to ensure that stderr is valid. What else can you do? You could grab a handle to "$CONOUT" but that would be annoying as it would override any piping a parent process is trying to do and write directly to the console, and would only work in the windows console, not mintty or other terminal emulators. Using MessageBox would be even more obnoxious and impractical.

nagisa · 2016-01-12T14:10:03Z

src/libstd/sys/unix/mod.rs

+    use intrinsics;
+    let msg = "fatal runtime error: out of memory\n";
+    unsafe {
+        libc::write(libc::STDERR_FILENO,


rtabort!?

See @alexcrichton's comment:

I'm a little hesitant about this as it's very tough to ensure there are 0 allocations along the way in the oom handler unless it's basically written from scratch. I would almost prefer that the handler in libstd is written in such a way that all the code is self-contained to be easily inspected for "this doesn't allocate". It'll be a little uglier, but much easier to audit and future-proof from accidental modifications

alexcrichton · 2016-01-12T16:58:59Z

@bors: r+ 03dd45a623b8deb1c528d4f65cda3023611cc119

bors · 2016-01-13T20:34:03Z

⌛ Testing commit 03dd45a with merge efbbe04...

bors · 2016-01-13T20:38:11Z

💔 Test failed - auto-win-msvc-64-opt

Amanieu · 2016-01-13T20:41:24Z

Oops, fixed.

nagisa · 2016-01-13T20:45:15Z

@bors r=alexcrichton 98bef2b

This adds the ability to override the default OOM behavior by setting a handler function. This is used by libstd to print a message when running out of memory instead of crashing with an obscure "illegal hardware instruction" error (at least on Linux). Fixes rust-lang#14674

- Successful merges: #30626, #30662, #30770, #30801, #30818, #30823, #30828, #30835, #30837, #30839, #30845, #30848, #30850, #30851, #30863 - Failed merges:

rust-highfive assigned alexcrichton Jan 9, 2016

Amanieu force-pushed the oom_print branch from 6aa5784 to fd00ff9 Compare January 10, 2016 19:50

Amanieu force-pushed the oom_print branch from fd00ff9 to 81e4102 Compare January 12, 2016 00:32

alexcrichton reviewed Jan 12, 2016
View reviewed changes

Amanieu force-pushed the oom_print branch from 81e4102 to f6e5337 Compare January 12, 2016 01:53

Add set_oom_handler and use it print a message when out of memory

757f57b

Amanieu force-pushed the oom_print branch from f6e5337 to 03dd45a Compare January 12, 2016 01:55

nagisa reviewed Jan 12, 2016
View reviewed changes

Add missing newline character to callers of dumb_print

98bef2b

Amanieu force-pushed the oom_print branch from 03dd45a to 98bef2b Compare January 13, 2016 20:40

Manishearth mentioned this pull request Jan 13, 2016

Rollup of 15 pull requests #30883

Merged

bors added a commit that referenced this pull request Jan 14, 2016

Auto merge of #30883 - Manishearth:rollup, r=Manishearth

5b3a75f

- Successful merges: #30626, #30662, #30770, #30801, #30818, #30823, #30828, #30835, #30837, #30839, #30845, #30848, #30850, #30851, #30863 - Failed merges:

bors merged commit 98bef2b into rust-lang:master Jan 14, 2016

Amanieu mentioned this pull request Apr 4, 2018

Add GlobalAlloc trait + tweaks for initial stabilization #49669

Merged

Amanieu mentioned this pull request May 29, 2018

OOM handling changes #50880

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Print a message on OOM #30801

Print a message on OOM #30801

Amanieu commented Jan 9, 2016

rust-highfive commented Jan 9, 2016

hanna-kruppe commented Jan 9, 2016

Amanieu commented Jan 9, 2016

Amanieu commented Jan 9, 2016

Amanieu commented Jan 9, 2016

Amanieu commented Jan 9, 2016

retep998 commented Jan 10, 2016

mmcco commented Jan 10, 2016

retep998 commented Jan 10, 2016

alexcrichton commented Jan 11, 2016

Amanieu commented Jan 11, 2016

retep998 commented Jan 11, 2016

Amanieu commented Jan 12, 2016

retep998 commented Jan 12, 2016

alexcrichton commented Jan 12, 2016

alexcrichton Jan 12, 2016

Amanieu commented Jan 12, 2016

alexcrichton Jan 12, 2016

alexcrichton commented Jan 12, 2016

Amanieu commented Jan 12, 2016

retep998 commented Jan 12, 2016

nagisa Jan 12, 2016

Amanieu Jan 12, 2016

nagisa Jan 12, 2016

alexcrichton commented Jan 12, 2016

bors commented Jan 13, 2016

bors commented Jan 13, 2016

Amanieu commented Jan 13, 2016

nagisa commented Jan 13, 2016

Print a message on OOM #30801

Print a message on OOM #30801

Conversation

Amanieu commented Jan 9, 2016

rust-highfive commented Jan 9, 2016

hanna-kruppe commented Jan 9, 2016

Amanieu commented Jan 9, 2016

Amanieu commented Jan 9, 2016

Amanieu commented Jan 9, 2016

Amanieu commented Jan 9, 2016

retep998 commented Jan 10, 2016

mmcco commented Jan 10, 2016

retep998 commented Jan 10, 2016

alexcrichton commented Jan 11, 2016

Amanieu commented Jan 11, 2016

retep998 commented Jan 11, 2016

Amanieu commented Jan 12, 2016

retep998 commented Jan 12, 2016

alexcrichton commented Jan 12, 2016

alexcrichton Jan 12, 2016

Choose a reason for hiding this comment

Amanieu commented Jan 12, 2016

alexcrichton Jan 12, 2016

Choose a reason for hiding this comment

alexcrichton commented Jan 12, 2016

Amanieu commented Jan 12, 2016

retep998 commented Jan 12, 2016

nagisa Jan 12, 2016

Choose a reason for hiding this comment

Amanieu Jan 12, 2016

Choose a reason for hiding this comment

nagisa Jan 12, 2016

Choose a reason for hiding this comment

alexcrichton commented Jan 12, 2016

bors commented Jan 13, 2016

bors commented Jan 13, 2016

Amanieu commented Jan 13, 2016

nagisa commented Jan 13, 2016